Skip to content

[Encryption] Improve diagnostic command #898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: feature/queryable-encryption
Choose a base branch
from
Open

[Encryption] Improve diagnostic command #898

wants to merge 1 commit into from
+493 −191

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jun 18, 2025 •
edited
Loading

From GromNaN#1

This improves the output of the diagnostics command and adds a command that prints the encrypted fields map for use in configuration.

Output:

❯ bin/console doctrine:mongodb:connection:diagnostic 

MongoDB Encryption Diagnostics
==============================

 PHP Environment
 * MongoDB extension loaded: Yes
 * MongoDB extension version: 1.21.1
 * MongoDB extension supports libmongocrypt: Yes
 * MongoDB library version: 2.1.0

 mongocryptd information
 * mongocryptd path: /Users/jerome/.local/bin/mongocryptd
 * mongocryptd version: mongocrypt version v8.0.8

Connection: default
-------------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Replica Set

 Auto Encryption Configuration
 * Auto Encryption Enabled: Yes
 * Key Vault Namespace: symfony.datakeys
 * Key Count: 0

                                                                                                         
 [OK] System looks ok for encryption support.

In case of incorrect setup:

MongoDB Encryption Diagnostics
==============================

 PHP Environment
 * MongoDB extension loaded: Yes
 * MongoDB extension version: 2.1.0
 * MongoDB extension supports libmongocrypt: Yes
 * MongoDB library version: 2.1.0

 mongocryptd information
 * mongocryptd: not found

Connection: default
-------------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Standalone

                                                                                                         
 [WARNING] This topology does not support encryption.                                                    
                                                                                                         

 Auto Encryption Configuration

                                                                                                         
 [ERROR] Failed to retrieve auto encryption information: mongocryptd error: No suitable servers found:   
         `serverSelectionTimeoutMS` expired: [connection refused calling hello on 'localhost:27020'].    
         Topology type: Single:                                                                          
                                                                                                         

Connection: other
-----------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Standalone

                                                                                                         
 [WARNING] This topology does not support encryption.                                                    
                                                                                                         

 Auto Encryption Configuration
 Auto encryption is not enabled for this connection.

                                                                                                         
 [WARNING] Not all requirements for encryption support are met. Please check the diagnostics above.

@GromNaN GromNaN changed the base branch from 5.4.x to feature/queryable-encryption June 18, 2025 09:32
@GromNaN GromNaN mentioned this pull request Jun 19, 2025
Closed
@GromNaN GromNaN force-pushed the improve-diagnostic-command branch from f75dee1 to 30a90d3 Compare June 19, 2025 20:17
@GromNaN GromNaN requested a review from alcaeus June 19, 2025 20:56
@GromNaN GromNaN marked this pull request as ready for review June 19, 2025 20:56
GromNaN
GromNaN commented Jun 19, 2025
View reviewed changes
src/Command/ConnectionDiagnosticCommand.php
} catch (Throwable $exception) {
$io->error('Could not retrieve auto encryption info: ' . $exception->getMessage());
}
$configOk = $this->printAndCheckConnectionDiagnostic($name, $diagnostic, $io) && $configOk;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When multiple connections are configured, but they don't have all auto encryption enabled, then the conclusion says there is an issue. We should return "OK" when auto encryption is not configured on a connection?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point. Indeed, if encryption wasn't configured on a connection, we should print an info line and mark it as "ok".

@GromNaN GromNaN force-pushed the improve-diagnostic-command branch from 9f8a963 to b4d1412 Compare June 19, 2025 21:37
@GromNaN GromNaN mentioned this pull request Jun 19, 2025
Draft
5 tasks
GromNaN
GromNaN commented Jun 23, 2025
View reviewed changes
src/DependencyInjection/DoctrineMongoDBExtension.php Outdated
Comment on lines 507 to 515
if (isset($driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'])) {
$fs = new Filesystem();
$cryptSharedLibPath = $driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'];

// If it's not an absolute path, resolve it relative to the project root
if (! $fs->isAbsolutePath($cryptSharedLibPath)) {
$driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'] = realpath($cryptSharedLibPath);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alcaeus I'm reverting this part. In Symfony we never convert relative paths to absolute.

I'll add documentation using %kernel.project_dir% instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - I wasn't quite sure if the kernel parameter was resolved properly. If that works, I prefer that over changing the path in the extension.

GromNaN
GromNaN commented Jun 23, 2025
View reviewed changes
src/Command/ConnectionDiagnosticCommand.php
{
public function __construct(
private readonly ServiceProviderInterface $diagnostics,
private readonly EncryptionDiagnostic $encryptionDiagnostic = new EncryptionDiagnostic(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't declare a service for EncryptionDiagnostic. Only leverage the default value from the constructor.

@GromNaN GromNaN force-pushed the improve-diagnostic-command branch from b4d1412 to e192526 Compare June 23, 2025 08:11
alcaeus
alcaeus reviewed Jun 23, 2025
View reviewed changes
@GromNaN GromNaN force-pushed the improve-diagnostic-command branch 2 times, most recently from 6b300d7 to cd2a184 Compare June 23, 2025 13:51
@alcaeus alcaeus requested a review from paulinevos June 25, 2025 15:23
@paulinevos
Copy link

There's a lot of commits in this. Does it need to be rebased?

@GromNaN
Copy link
Member Author

GromNaN commented Jun 26, 2025

There's a lot of commits in this. Does it need to be rebased?

It will be squashed on merge. All the commits represent the iterations on this PR.

paulinevos
paulinevos requested changes Jun 26, 2025
View reviewed changes
Copy link

@paulinevos paulinevos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small comments, otherwise seems fine

@GromNaN GromNaN force-pushed the improve-diagnostic-command branch from a9805b3 to fd27ff0 Compare June 30, 2025 12:45
@GromNaN GromNaN requested a review from paulinevos June 30, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcaeus alcaeus alcaeus left review comments

@paulinevos paulinevos Awaiting requested review from paulinevos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GromNaN @paulinevos @alcaeus